feat: add Ctrl-] x3 hotkey to power cycle board from serial console - #791
feat: add Ctrl-] x3 hotkey to power cycle board from serial console#791evakhoni wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable power-cycle support and reconnect handling to the pyserial console. ChangesSerial Console Power-Cycle and Reconnect
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LuuOW
left a comment
There was a problem hiding this comment.
Technical audit: Implementation verified for architectural consistency and engineering integrity.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py`:
- Around line 135-142: The `_search_power()` method only checks the children of
the provided client node but skips checking if the client node itself is
power-capable. This causes power-driver discovery to fail when the root node has
power capabilities. Modify the method to first check if the current `client`
parameter has a `cycle` method or both `on` and `off` methods before recursing
into `client.children`, ensuring the root node is included in the power-driver
discovery process.
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py`:
- Around line 17-37: The slave_file opened via os.fdopen() in the
_start_console() function is never explicitly closed, causing file descriptor
leaks across multiple test runs. Ensure proper cleanup by either returning
slave_file as part of the function's return tuple so the caller can close it
after the thread completes, or by adding a cleanup mechanism that closes
slave_file after the console.run() thread finishes executing. The key is to
guarantee that the file handle opened for slave_fd is properly closed to prevent
descriptor exhaustion.
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.py`:
- Around line 47-52: The exception re-raising in the except* ConsoleStreamDrop
and except EndOfStream blocks violates Ruff B904 linting rules by not using
explicit exception chaining. For each exception handler block that raises an
exception (the except* ConsoleStreamDrop block that raises ConsoleStreamDrop()
and the except EndOfStream block that raises ConsoleStreamDrop()), capture the
caught exception as a variable in the except clause (e.g., except*
ConsoleStreamDrop as exc) and use the from keyword when raising to provide
explicit chaining (e.g., raise ConsoleStreamDrop() from exc). Alternatively, use
make lint-fix to automatically apply the necessary fixes to satisfy B904
requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c042f0b-f525-40fa-9c8d-e51bfe0279df
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.pypython/packages/jumpstarter/jumpstarter/client/client.py
67ebd74 to
beac1ee
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py (1)
8-33: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConsolidate new driver-package tests into
driver_test.pyper repository policy.Both files add comprehensive driver-package test coverage, but not in the required module location.
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py#L8-L33: move/merge the power discovery and power-cycle tests intojumpstarter_driver_pyserial/driver_test.py.python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py#L40-L91: move/merge the Ctrl-B/Ctrl-] integration tests intojumpstarter_driver_pyserial/driver_test.py.As per coding guidelines,
**/jumpstarter-driver-*/jumpstarter_driver_*/*_test.py: Add comprehensive tests indriver_test.pyfile within the driver package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py` around lines 8 - 33, Move the power discovery and power-cycle tests (test_find_power_client_no_root, test_find_power_client_with_cycle, test_make_power_cycle_calls_cycle) from python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py lines 8-33 into python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver_test.py, and similarly move the Ctrl-B/Ctrl-] integration tests from python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py lines 40-91 into the same driver_test.py file. Per the coding guidelines, comprehensive driver-package tests must be consolidated in the driver_test.py file within the driver package rather than scattered across individual module test files.Source: Coding guidelines
🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py (1)
24-33: ⚡ Quick winAdd coverage for the
_make_power_cyclefallback branch.
test_make_power_cycle_calls_cyclevalidates only thecycle()path; theoff()→sleep(2)→on()path inpython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pyis still untested.💡 Suggested test addition
+def test_make_power_cycle_calls_off_on_when_cycle_missing(): + called_off = threading.Event() + called_on = threading.Event() + power = MagicMock(spec=["off", "on"]) + power.off = MagicMock(side_effect=lambda: called_off.set()) + power.on = MagicMock(side_effect=lambda: called_on.set()) + + with serve(PySerial(url="loop://")) as client: + cycle_fn = client._make_power_cycle(power) + client.portal.call(cycle_fn) + + assert called_off.is_set() + assert called_on.is_set()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py` around lines 24 - 33, The test_make_power_cycle_calls_cycle test only covers the cycle() code path in the _make_power_cycle method, but the fallback branch that uses off() followed by sleep(2) followed by on() remains untested. Add a new test case that mocks the power object to trigger the fallback path (either by not providing a cycle method or having it unavailable), then verify that the fallback sequence of off(), sleep(), and on() method calls is executed properly when the cycle function is invoked through client.portal.call().python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py (1)
44-45: ⚡ Quick winReplace fixed sleeps with deterministic readiness signaling.
Line 44, Line 64, and Line 81 use
time.sleep(0.1)to assume console readiness; this can cause intermittent CI flakes.Also applies to: 64-65, 81-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py` around lines 44 - 45, Replace the three instances of time.sleep(0.1) calls with deterministic readiness signaling instead of relying on fixed delays to assume console readiness. For each location where time.sleep(0.1) precedes an os.write operation, implement a proper synchronization mechanism such as using select, poll, or event-based signaling to confirm the file descriptor is actually ready for writing before proceeding, eliminating the intermittent CI flakes caused by timing assumptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py`:
- Around line 8-33: Move the power discovery and power-cycle tests
(test_find_power_client_no_root, test_find_power_client_with_cycle,
test_make_power_cycle_calls_cycle) from
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py
lines 8-33 into
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver_test.py,
and similarly move the Ctrl-B/Ctrl-] integration tests from
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py
lines 40-91 into the same driver_test.py file. Per the coding guidelines,
comprehensive driver-package tests must be consolidated in the driver_test.py
file within the driver package rather than scattered across individual module
test files.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py`:
- Around line 24-33: The test_make_power_cycle_calls_cycle test only covers the
cycle() code path in the _make_power_cycle method, but the fallback branch that
uses off() followed by sleep(2) followed by on() remains untested. Add a new
test case that mocks the power object to trigger the fallback path (either by
not providing a cycle method or having it unavailable), then verify that the
fallback sequence of off(), sleep(), and on() method calls is executed properly
when the cycle function is invoked through client.portal.call().
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py`:
- Around line 44-45: Replace the three instances of time.sleep(0.1) calls with
deterministic readiness signaling instead of relying on fixed delays to assume
console readiness. For each location where time.sleep(0.1) precedes an os.write
operation, implement a proper synchronization mechanism such as using select,
poll, or event-based signaling to confirm the file descriptor is actually ready
for writing before proceeding, eliminating the intermittent CI flakes caused by
timing assumptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb30cdcc-eb35-42ef-8a00-b909cd4dc09f
📒 Files selected for processing (2)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/console_test.py
|
|
||
| return bytes_read, bytes_sent | ||
|
|
||
| def _find_power_client(self): |
There was a problem hiding this comment.
I think we should provide a config entry to let admins specify the power device via a "ref". While I think that finding it automatically is cool, and will work out of the box in most cases, imagine environments where you have multiple power controls , and the power controls could not be what you are expecting, or the reset mechanism is different.
Even in some cases the method to be called could be "reset()", i.e. in the esp32 controller.
So I would take a config with "ref" + method in the serial config
There was a problem hiding this comment.
i.e.
export:
serial:
type:....
config:
....
reset_control:
device:
ref: "esp32"
commands:
- "reset()"
export:
serial:
type:....
config:
....
reset_control:
device:
ref: "power"
commands:
- "on()"
- "sleep 2"
- "off()"
or something like this.
There was a problem hiding this comment.
yeah config sounds good. but if unset let it fallback to the auto discovery WDYT? otherwise I guess the user base would be rather small..
+1 on the reset(), i thought I seen it somewhere but forgot it was esp32.
There was a problem hiding this comment.
IMHO auto detect continues to be risky, it could pick up the wrong power device if you had multiple doing different things in your setup. We could provide a flag for auto-detection, but explain very clearly what it does, and should be disabled by default.
There was a problem hiding this comment.
or even enabled by default.. but a flag :D
There was a problem hiding this comment.
Yep, that's a valid concern. I think I found a middle ground: conservative auto-discovery that disables on first trouble + explicit config when needed.
Added power_control_ref and power_control_method fields — you can now specify which device and what sequence to call:
power_control_ref: "esp32"
power_control_method: ["hard_reset"]Auto-discovery now uses a strict allowlist (only PowerClient/VirtualPowerClient labels) and disables itself if it finds multiple candidates. No flag needed since it's conservative by design.
Ready for re-review when you have time!
There was a problem hiding this comment.
Btw if needed - there's an option to disable it via the config, by setting
- power_control_method: [] ✅
- power_control_method: "" ✅
- power_control_method: null ✅
|
|
||
| return bytes_read, bytes_sent | ||
|
|
||
| def _find_power_client(self): |
There was a problem hiding this comment.
or even enabled by default.. but a flag :D
Replace duck-typing power discovery with conservative label-based detection to prevent false positives (GPIO pins, storage mux drivers) and handle multi-power setups safely. Changes: - Add power_control_ref config field for explicit device selection - Add power_control_method config field for custom sequences (default: ["cycle"]) - Implement allowlist: only PowerClient/VirtualPowerClient auto-discovered - Add ambiguity detection: warn and disable when multiple candidates found - Support method sequences: ["off", "sleep:2", "on"], ["reset"], etc. - Support disable via empty list: power_control_method: [] - Pass config to client via extra_labels() following DbusNetwork pattern Addresses reviewer feedback on PR jumpstarter-dev#791 about auto-discovery risks in multi-power environments. Feature remains enabled by default for simple setups but disables itself at first sign of trouble. Tests: 71/71 pass, lint and type checks clean Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py (1)
13-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest
VirtualPowerClientdiscovery.This test covers
PowerClientonly.KNOWN_POWER_CLIENTSalso acceptsVirtualPowerClient. Add a parameterized case for"jumpstarter_driver_power.client.VirtualPowerClient"so a regression in that supported label is detected.As per coding guidelines,
python/**/*_test.pymust “Provide comprehensive package test coverage.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py` around lines 13 - 23, Extend test_find_power_client_auto_discover to cover both supported client labels by parameterizing the test and including "jumpstarter_driver_power.client.VirtualPowerClient" alongside the existing PowerClient label. Keep the discovery setup and assertion unchanged for each parameterized case.Source: Coding guidelines
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the annotation with the accepted input.
Line [103] declares
power_control_methodaslist[str], but Lines [112-115] accept astr. A typed caller cannot pass a supported string value without a type-checking error. Use a union at the input boundary, then normalize the stored value tolist[str].Also applies to: 112-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py` at line 103, Update the power_control_method annotation and initialization in the driver configuration to accept both str and list[str] at the input boundary, matching the handling in lines 112-115. Normalize a supplied string to a one-element list so the stored value remains list[str], while preserving the existing default and list behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py`:
- Around line 188-204: Update the power-control setup around _cycle to validate
every step before enabling the hotkey: require non-sleep operations on
power_client to be callable, parse each sleep: delay during construction, and
catch invalid values by logging a warning and returning None. Preserve valid
operation ordering, and add tests covering a non-callable attribute and an
invalid sleep value.
---
Outside diff comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.py`:
- Around line 13-23: Extend test_find_power_client_auto_discover to cover both
supported client labels by parameterizing the test and including
"jumpstarter_driver_power.client.VirtualPowerClient" alongside the existing
PowerClient label. Keep the discovery setup and assertion unchanged for each
parameterized case.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py`:
- Line 103: Update the power_control_method annotation and initialization in the
driver configuration to accept both str and list[str] at the input boundary,
matching the handling in lines 112-115. Normalize a supplied string to a
one-element list so the stored value remains list[str], while preserving the
existing default and list behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eeb0681c-6d5e-451d-b312-e5f4f45568d6
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client_test.pypython/packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py
Replace duck-typing power discovery with conservative label-based detection to prevent false positives (GPIO pins, storage mux drivers) and handle multi-power setups safely. Changes: - Add power_control_ref config field for explicit device selection - Add power_control_method config field for custom sequences (default: ["cycle"]) - Implement allowlist: only PowerClient/VirtualPowerClient auto-discovered - Add ambiguity detection: warn and disable when multiple candidates found - Support method sequences: ["off", "sleep:2", "on"], ["reset"], etc. - Support disable via empty list [], empty string "", or null - Pass config to client via extra_labels() following DbusNetwork pattern - Pre-validate callable methods and parse sleep values at construction - Fix type annotation: power_control_method accepts list[str] | str | None - Add field docstrings and update README with config table and hotkey docs Addresses reviewer feedback on PR jumpstarter-dev#791 about auto-discovery risks in multi-power environments and CodeRabbit findings on validation timing. Feature remains enabled by default for simple setups but disables itself at first sign of trouble. Tests: 74/74 pass, coverage improved from 78% to above 80% threshold Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ae02650 to
ea0e41a
Compare
- Add on_power_cycle callback to Console; Ctrl-] x3 triggers it - Add root back-reference to DriverClient, stamped during tree construction - Auto-discover power client from DUT siblings in PySerialClient - Prefer cycle() when available, fall back to off()+on() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ConsoleStreamDrop exception to signal a dropped serial stream - Add retry loop in start_console to reconnect on stream drop - Use object.__setattr__ to stamp root back-reference without polluting pydantic schema - Remove root field from DriverClient; it is now a dynamic attribute Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Ctrl-B x3 exits cleanly - Ctrl-] x3 triggers on_power_cycle and console stays alive - Ctrl-] x3 without a power client does nothing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace duck-typing power discovery with conservative label-based detection to prevent false positives (GPIO pins, storage mux drivers) and handle multi-power setups safely. Changes: - Add power_control_ref config field for explicit device selection - Add power_control_method config field for custom sequences (default: ["cycle"]) - Implement allowlist: only PowerClient/VirtualPowerClient auto-discovered - Add ambiguity detection: warn and disable when multiple candidates found - Support method sequences: ["off", "sleep:2", "on"], ["reset"], etc. - Support disable via empty list [], empty string "", or null - Pass config to client via extra_labels() following DbusNetwork pattern - Pre-validate callable methods and parse sleep values at construction - Fix type annotation: power_control_method accepts list[str] | str | None - Add field docstrings and update README with config table and hotkey docs Addresses reviewer feedback on PR jumpstarter-dev#791 about auto-discovery risks in multi-power environments and CodeRabbit findings on validation timing. Feature remains enabled by default for simple setups but disables itself at first sign of trouble. Tests: 74/74 pass, coverage improved from 78% to above 80% threshold Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes two power auto-discovery bugs: 1. **Proxy dedup**: When a Proxy driver delegates to a power client (e.g., Proxy(ref="qemu.power")), the same driver instance appears twice in the client tree — once via proxy, once via direct parent. This triggered false "Multiple power drivers found" ambiguity. Fixed by tracking UUIDs in `_collect_power_clients` to skip already-seen drivers. 2. **Allowlist expansion**: RideSXPowerClient, NoyitoPowerClient, and SNMPServerClient all extend PowerClient but weren't in the allowlist, causing auto-discovery to silently find nothing on those targets. Added all three to `KNOWN_POWER_CLIENTS`. Also adds user feedback: when Ctrl-] x3 is pressed without a power driver, log a warning instead of silently doing nothing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ea0e41a to
f807a7f
Compare
Summary
power_control_ref(explicit device name) andpower_control_method(method sequence with sleep support)power_control_methodto[]ornullto disable explicitlyHow it works
The root client is back-referenced onto every driver client after tree construction.
PySerialClientwalks the tree matchingjumpstarter.dev/clientlabels against a known-power-clients allowlist. If exactly one match is found, it's used; multiple matches disable the hotkey with a warning. The method sequence is pre-validated at console start — invalid methods or malformedsleep:values disable the hotkey immediately rather than failing at press time. Configuration is passed from exporter to client viaextra_labels().Test plan
power_control_refset to explicit device: uses that device directlypower_control_method: nullor[]: hotkey disabled